Adds a ReactNativeInspector API to ReactNativeRenderer by trueadm · Pull Request #9691 · facebook/react

trueadm · 2017-05-15T16:34:36Z

Currently, React Native has internal implementation details of how Fiber and Stack work in order to make the built-in React Native Inspector tool. This PR aims at adding an API to ReactNativeRenderer so React Native can access the internals via an externally made public API. This API is DEV only, so should not affect file size in production.

An upstream React Native PR will follow too, adding support for React Native to use this new API.

…tor_api

sebmarkbage · 2017-05-15T22:07:46Z

+    const fiber = ReactNativeComponentTree.findFiberByHostInstance(viewTag);
+    const hierarchy = getOwnerHierarchy(fiber);
+    const instance = lastNotNativeInstance(hierarchy);
+    const props = instance.memoizedProps || {};


This is not guaranteed to yield the current props. This could've been the previous or next props if the fiber we're getting is not the current one.

getFiberCurrentPropsFromNode is also not guaranteed to have the current props, because it should only be used for event listeners (atm) and therefore only updated if event listeners change.

So what implementation is right here then? This was ported from an old PR, so it would be good to know what the ideal would be here.

I'm not sure. :) The way we solved it in the devtools is that we track everything that could've been updated but it seems like overkill to do here.

We could use ReactFiberTreeReflection.findCurrentFiberUsingSlowPath. That one will guarantee to yield the current Fiber and therefore the correct props for this particular Fiber but won't yield the correct props for every Fiber on the way up to the root owner.

We could call it repeatedly for every Fiber on the way to the root. Maybe that's not too bad for this use case?

sebmarkbage · 2017-05-15T22:09:17Z

+
+  var getInspectorDataForViewTag = function(viewTag: any): Object {
+    const fiber = ReactNativeComponentTree.findFiberByHostInstance(viewTag);
+    const hierarchy = getOwnerHierarchy(fiber);


This yields a stack of Fibers, right? What do you do with them on the other side? Can we expose something less leaky, like an object such as the one returned by this function.

This is needed by inspector to show the current position in the tree relative to the root. It expects an array of fibers here.

trueadm · 2017-05-22T14:23:38Z

@sebmarkbage can you take another look on this PR please?

…erstoof

bvaughn

Some tiny changes that we talked about in person. Deferring to @sebmarkbage for final say though.

bvaughn · 2017-05-22T15:47:19Z

+        hostNode: getHostNode(fiber, findNodeHandle),
+        props: fiber.stateNode
+          ? getFiberCurrentPropsFromNode(fiber.stateNode)
+          : {},


Should we use emptyObject here?

This is not guaranteed to have the current props. It's misnamed. It's only guaranteed to have the current event handlers. ReactFiberTreeReflection.findCurrentHostFiber(fiber).memoizedProps is better for inspection.

bvaughn · 2017-05-22T15:57:43Z

+    const componentHierarchy = getOwnerHierarchy(component);
+    const instance = lastNotNativeInstance(componentHierarchy);
+    const hierarchy = createHierarchy(componentHierarchy);
+    const props = (instance._instance || {}).props || {};


empty object :D

gaearon · 2017-05-22T16:24:26Z

+  var lastNotNativeInstance = function(hierarchy) {
+    for (let i = hierarchy.length - 1; i > 1; i--) {
+      const instance = hierarchy[i];
+      if (!instance.viewConfig) {


I don't think this check would work with Fiber. Would it? hierarchy contains fibers which wouldn't have the viewConfig property.

gaearon · 2017-05-22T16:30:19Z

+    // look for children first for the hostNode
+    // as composite fibers do not have a hostNode
+    while (fiber) {
+      hostNode = findNodeHandle(fiber.stateNode);


Can we first check that fiber type is either ClassComponent or HostComponent? I don't think it's universally safe to take .stateNode and expect it to be either a class or a node. For example coroutines also use .stateNode for a different purpose, and findNodeHandle() would blow up on unexpected inputs.

I would imagine something like:

if (fiber.stateNode !== null && (fiber.type === HostComponent || fiber.type === ClassComponent)) { hostNode = findNodeHandle(fiber.stateNode); }

I’m actually not sure if we even need to check for ClassComponent there because we’d drill down anyway. So maybe it’s enough to check fiber.type === HostComponent && fiber.stateNode !== null.

WDYT?

We shouldn't even expose stateNode of HostComponent. That data structure will need to change and likely turn into something native. In fact, it may just be a number.

I'm confused with the check on fiber.type, when changing it to either HostComponent or ClassComponent, it fails as it never matches these, it seems they are functions not numbers.

sebmarkbage · 2017-05-22T16:38:04Z

Do you have an equivalent PR coming for React Native so that we can see how you intend to use this API?

I don't think you should need to expose any Fibers or the stateNode from a HostComponent. All of that can have wrapper objects that you expose from here to the UI part and let the UI part just a dumb view that presents that in Inspector.js. This is the minimal to expose. Although this might not even be ideal. It's just what whoever built-it first thought about.

gaearon · 2017-05-22T16:40:37Z

I don't think Fibers are exposed anymore:

https://github.com/facebook/react/pull/9691/files#diff-a34363327f035a330f6a579ba383b2a4R59

They're mapped to opaque objects. I guess putting measure() on those would let us get rid of exposing host nodes?

sebmarkbage · 2017-05-22T16:42:01Z

+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ *
+ * @providesModule ReactNativeFiberInspector


I think the value added by this UI is that you can inspect a quick breadcrumb - on the device. Otherwise you can just use the full DevTools.

There's nothing inherently ReactNative specific about this type of UI. Mobile web could use the same. We should build the same thing for web if it is useful to have a UI that just quickly shows the breadcrumb.

As a start, can we move this to "shared" and call it something more generic like ReactFiberBreadCrumbInspector.

Is this something that is going to add a lot of value to do now rather than later? I get the reasoning for doing this, but I feel having a working Fiber inspector merged soon along with RN flat bundles is going to be of more value. What do you think?

I agree I’d prefer we do another pass at this later, but unblock RN Fiber rollout now.
(After fixing the other issues where we seem very close)

Fixes a flow error

…ure out

gaearon · 2017-05-23T17:32:30Z

+    }));
+  };
+
+  const {getClosestInstanceFromNode} = ReactNativeComponentTree;


Let's move imports to the top.

gaearon · 2017-05-23T17:32:42Z

+
+  const {findCurrentFiberUsingSlowPath} = ReactFiberTreeReflection;
+
+  var getInspectorDataForViewTag = function(viewTag: number): Object {


Let's make sure it's still defined in PROD mode (but is throwing).

gaearon · 2017-05-23T17:34:54Z

+    }));
+  };
+
+  var getInspectorDataForViewTag = function(viewTag: any): Object {


Let's make sure this is defined in PROD too.

gaearon · 2017-05-23T17:35:47Z

+
+  var getHostProps = function(fiber) {
+    return (ReactFiberTreeReflection.findCurrentHostFiber(fiber) || emptyObject)
+      .memoizedProps;


Nit: let's make the intent more explicit by doing a check in a ternary or an if statement instead of reading emptyObject.memoizedProps.

gaearon · 2017-05-23T17:38:08Z

+      getInspectorData: () => ({
+        measure: callback =>
+          UIManager.measure(component.getHostNode(), callback),
+        props: (component._instance || emptyObject).props || emptyObject,


Nit: let's make the intent more explicit by doing a check in a ternary or an if statement instead of reading emptyObject.props.

gaearon · 2017-05-23T17:38:19Z

+    const componentHierarchy = getOwnerHierarchy(component);
+    const instance = lastNotNativeInstance(componentHierarchy);
+    const hierarchy = createHierarchy(componentHierarchy);
+    const props = (instance._instance || emptyObject).props || emptyObject;


Nit: let's make the intent more explicit by doing a check in a ternary or an if statement instead of reading emptyObject.props.

…n feedback

Summary: This PR aims to update the Inspector tool in React Native to use the new inspection APIs that have been added to the ReactNative renderer: facebook/react#9691 This PR also cleans up the code in `Inspector.js` so there's no usage of React's internals. Closes #14160 Reviewed By: bvaughn Differential Revision: D5129280 Pulled By: trueadm fbshipit-source-id: b1b077c04f46b0f52cdea0e19b4154441558f77a

fixed conflicts with master

ddfd05f

facebook-github-bot added the CLA Signed label May 15, 2017

trueadm added 3 commits May 15, 2017 17:40

splits fiber and stack implementations

12e8367

prettier run

9cb93e5

Merge remote-tracking branch 'upstream/master' into reactnativeinspec…

87d5fa3

…tor_api

sebmarkbage reviewed May 15, 2017

View reviewed changes

trueadm added 4 commits May 16, 2017 13:38

updated fiber implementation based on feedback

179926e

updated fiber implementation for selecting inspector hierarchy

6edea82

run of prettier

416c207

fixed flow issues

532912a

trueadm changed the title ~~[WIP] Adds a ReactNativeInspector API to ReactNativeRenderer~~ Adds a ReactNativeInspector API to ReactNativeRenderer May 22, 2017

trueadm requested review from acdlite, bvaughn and gaearon May 22, 2017 10:24

updated stack implementation

159efdb

fixes an implementation difference where before it wasnt properly und…

3a3d8c3

…erstoof

bvaughn reviewed May 22, 2017

View reviewed changes

gaearon reviewed May 22, 2017

View reviewed changes

sebmarkbage reviewed May 22, 2017

View reviewed changes

trueadm and others added 7 commits May 23, 2017 13:29

addresses comments in PR feedback

ba9c3fa

updated stack inspector to use emptyObject

28bef25

Update ReactNativeFiberInspector.js

0f88fa5

Fixes a flow error

fixes last flow error

450da60

prettier

6215bf9

applied changes to how viewConfig works for fibers and extracted meas…

d378e9c

…ure out

fixed bad paste

76b721a

trueadm added 3 commits May 23, 2017 17:56

fixes flow errors

4ce3fc8

prettyify

fc0c137

revmoed getComponentName changes

a6f8e6b

gaearon reviewed May 23, 2017

View reviewed changes

updated logic for getHostProps and addressed other PR feedback

3e84ec9

gaearon approved these changes May 23, 2017

View reviewed changes

trueadm added 2 commits May 23, 2017 18:46

improved throwing to invariant and update stack implemenation based o…

e0e443e

…n feedback

prettier

b05ef94

trueadm merged commit 0ea4674 into facebook:master May 23, 2017

trueadm mentioned this pull request May 24, 2017

Refactor of Fiber integration with React Fiber + Stack facebook/react-native#14160

Closed

gaearon deleted the reactnativeinspector_api branch May 25, 2017 16:27


		const {findCurrentFiberUsingSlowPath} = ReactFiberTreeReflection;

		var getInspectorDataForViewTag = function(viewTag: number): Object {

Conversation

trueadm commented May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trueadm commented May 22, 2017

Uh oh!

bvaughn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage commented May 22, 2017

Uh oh!

gaearon commented May 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

trueadm commented May 15, 2017 •

edited

Loading

sebmarkbage May 15, 2017 •

edited

Loading

gaearon May 22, 2017 •

edited

Loading